-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update tau anti electron discriminators in RECO and MiniAOD #27465
Update tau anti electron discriminators in RECO and MiniAOD #27465
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27465/10792
|
A new Pull Request was created by @swozniewski for master. It involves the following packages: PhysicsTools/NanoAOD @perrotta, @cmsbuild, @fgolf, @slava77, @santocch, @peruzzim can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the implementations to change WPeffXX to WPeffYY appear to be extremely verbose.
Unless I missed some irregularity in the pattern, it seems more practical to compactify
hpsPFTauDiscriminationByMVA6LooseElectronRejection.mapping[5].cut = cms.string("RecoTauTag_antiElectronMVA6v1_gbr_NoEleMatch_wGwoGSF_EC_WPEff96") | ||
hpsPFTauDiscriminationByMVA6LooseElectronRejection.mapping[6].cut = cms.string("RecoTauTag_antiElectronMVA6v1_gbr_woGwGSF_EC_WPEff96") | ||
hpsPFTauDiscriminationByMVA6LooseElectronRejection.mapping[7].cut = cms.string("RecoTauTag_antiElectronMVA6v1_gbr_wGwGSF_EC_WPEff96") | ||
hpsPFTauDiscriminationByMVA6LooseElectronRejection.mapping[0].cut = cms.string("RecoTauTag_antiElectronMVA6v3_noeveto_gbr_NoEleMatch_woGwoGSF_BL_WPeff90") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't all of this be rolled up in a simple loop? smth like
for m in hpsPFTauDiscriminationByMVA6LooseElectronRejection.mapping :
m.cut = m.cut.replace("WPeff98", "WPeff90")
) | ||
# Loose | ||
patTauDiscriminationByLooseElectronRejectionMVA62015 = patTauDiscriminationByVLooseElectronRejectionMVA62015.clone( | ||
mapping = cms.VPSet( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't all of the below be rolled up in a simple loop? smth like
patTauDiscriminationByLooseElectronRejectionMVA62015 = patTauDiscriminationByVLooseElectronRejectionMVA62015.clone()
for m in patTauDiscriminationByLooseElectronRejectionMVA62015.mapping :
m.cut = m.cut.replace("WPeff99", "WPeff96")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, why not. Update follows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cms.string does not seem to support replace or conversion to python string. Do you have an idea, how to do this in an elegant way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it gets too ugly, we can also wait for the new tau discriminator format, where these WPs and categories will be separated in the configurations and then 'multiplied' by the module. https://github.com/swozniewski/cmssw/blob/restructure_PFTauDiscriminators_v3/RecoTauTag/Configuration/python/HPSPFTaus_cff.py#L320-L376
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.pythonValue may be needed on rhs (before .replace)
https://github.com/cms-sw/cmssw/blob/master/FWCore/ParameterSet/python/Types.py#L300
@Dr15Jones @makortel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint, this could be used! However, this function puts the string in single quotes, which I have to remove:
m.cut = m.cut.pythonValue().strip("'").replace("WPeff98", "WPeff90")
Does not look very nice but would work...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is also a .configValue return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use m.cut.value()
which returns the held python string. Remember, you can use python to look at any of the classes to find what methods they have
bash>python
>>> import FWCore.ParameterSet.Config as cms
>>> help(cms.string)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks good
Hello Marco,
I think it is impossible to avoid such "cascade" PRs if changes are introduced to any pre-Nano level in case they were already intdouced to NanoAOD. I think it happens because release cycle of Nano is shorther than for AOD and MiniAOD.
Nothing, i.e. no backport. It is as changes in AOD/MiniAOD are not foreseen/allowed for closed releases. Finally, Tau POG plans to clear NanoAOD from outdated Run-2 tauIDs, but it is beyond scope of this PR (and will be performed via NanoAOD repo) |
Thanks for clarifying. I am going to test this and then sign, but I don't foresee any issues. |
@peruzzim |
@peruzzim any news about this? |
ping |
+xpog I have not detected changes that would break the existing setup, the status of discriminants stored in nanoAOD for different eras will be rediscussed for the future when x-checking the sync between 11_0 and previous releases. |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
No, backport of this is not expected. There are two reasons: the main one is the fact that DeepTauID overperforms all current BDT-based TauIDs (including anti-e 2018) so it is going to be the main POG recomended set of TauID, the other reason is that anti-e MVA 2018 is already available for users via NanoAOD (or can be run on top of MiniAOD by whose are not using NanoAOD). |
I'm curious if this expectation changed since Sep 20 of last year. |
I think that the issue is related with the fact that for some time payloads of BDT-based tauIDs (including |
Thank you for clarifying that the main difference is in the payloads. |
After some thinking, we find that it makes sense to update |
PR description:
This PR updates the version of the tau AntiElectronDiscriminator in RECO and miniAOD to the latest 2018 version. This is to have the latest tau discriminator versions ready for a coming integration of tau ID versions into global tag.
The NanoAOD config is updated such that 2018 version is taken from PAT taus and the old 2015 version is recalculated. For already existing input samples from former productions a switch is introduced to do it the other way round (as it was before this PR).
PR validation:
Reco and PAT step were run on a test set and output compared to corresponding NanoAOD sample that already contains both versions of AntiEleDiscriminator. NanoAOD step was also run on a test set with different config eras and output checked.